Skip to content

Fix disconnect_expired_cert when Kube Identity forwarding is used#24913

Merged
tigrato merged 2 commits intomasterfrom
tigrato/fix-disconnect_expired_cert
Apr 24, 2023
Merged

Fix disconnect_expired_cert when Kube Identity forwarding is used#24913
tigrato merged 2 commits intomasterfrom
tigrato/fix-disconnect_expired_cert

Conversation

@tigrato
Copy link
Copy Markdown
Contributor

@tigrato tigrato commented Apr 20, 2023

Teleport 13 introduces the identity forwarding mechanism that allows a proxy to forward the client's identity without re-signing a new certificate on his behalf. Proxy uses its certificate key pair and it's valid for a long period of time resulting in the current version not respecting the connection termination.

This PR removes the parsing of the connection certificate and uses the value provided by the unmapped identity - supporting the new and old forwarding methods.

Fixes #24910

Teleport 13 introduces the identity forwarding mechanism that allows
a proxy to forward the client's identity without re-signing a new
certificate on his behalf. Proxy uses its certificate key pair and it's
valid for a long period of time resulting in the current version not
respecting the connection termination.

This PR removes the parsing of the connection certificate and uses the
value provided by the unmapped identity - supports the new and old
forwarding methods.

Fixes #24910
RouteToCluster: tt.routeToCluster,
KubernetesCluster: tt.kubernetesCluster,
ActiveRequests: tt.activeRequests,
Expires: certExpiration,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I add this line, but don't add the fixes above, the test still passes.

This makes me think we don't actually have a test that verifies this behavior is correct.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was a nice catch although the test was correct. I didn't remove the expiration from the certificate and it was picking that value.
Fixed by 1cbc54b

Copy link
Copy Markdown
Contributor

@ibeckermayer ibeckermayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not quite clear on what identity is what here, but I notice that you say

This PR removes the parsing of the connection certificate and uses the value provided by the unmapped identity - supporting the new and old forwarding methods.

but you are only using something called UnmappedIdentity in the kubeResourceDeniedAccessMsg. In setupContext, you're using identity := authCtx.Identity.GetIdentity(). Probably a definitional matter, but just double checking.

@tigrato
Copy link
Copy Markdown
Contributor Author

tigrato commented Apr 20, 2023

I'm not quite clear on what identity is what here, but I notice that you say

This PR removes the parsing of the connection certificate and uses the value provided by the unmapped identity - supporting the new and old forwarding methods.

but you are only using something called UnmappedIdentity in the kubeResourceDeniedAccessMsg. In setupContext, you're using identity := authCtx.Identity.GetIdentity(). Probably a definitional matter, but just double checking.

When I refer unmapped identity what I wanted to say is: the value received from the unmapped identity in the TLS certificate or the unmapped value received as impersonation. For both cases, the unmapped expire is adjusted with the identity roles rules after mapping the identity to local. That's why I use the mapped identity (Identity) to get the value.

Sorry for the confusion

@tigrato tigrato added this pull request to the merge queue Apr 24, 2023
Merged via the queue into master with commit 8558529 Apr 24, 2023
@tigrato tigrato deleted the tigrato/fix-disconnect_expired_cert branch April 24, 2023 09:18
@public-teleport-github-review-bot
Copy link
Copy Markdown

@tigrato See the table below for backport results.

Branch Result
branch/v13 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Kubernetes identity forwarding broke disconnect_expired_cert

4 participants